Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.x] Alpha ordered imports #29933

Merged
merged 2 commits into from
Sep 10, 2019
Merged

[6.x] Alpha ordered imports #29933

merged 2 commits into from
Sep 10, 2019

Conversation

driesvints
Copy link
Member

giphy-downsized-large

@antonkomarev
Copy link
Contributor

Finally!

@GrahamCampbell
Copy link
Member

I kinda liked the ordered imports in Laravel, even though I alpha order my own code. :P

@browner12
Copy link
Contributor

I'm with Graham. I don't see any valuable reason to change.

@antonkomarev
Copy link
Contributor

antonkomarev commented Sep 10, 2019

I could see one valuable reason for a future: alpha ordered imports could be easily groupped.

Instead:

use Illuminate\Contracts\Auth\Authenticatable as UserContract;
use Illuminate\Contracts\Auth\UserProvider;
use Illuminate\Contracts\Hashing\Hasher as HasherContract;

You could have:

use Illuminate\Contracts\{
    Auth\Authenticatable as UserContract,
    Auth\UserProvider,
    Hashing\Hasher as HasherContract
};

@browner12
Copy link
Contributor

why is the grouping valuable?

@driesvints
Copy link
Member Author

I'm not a fan of grouping myself 😅

@antonkomarev
Copy link
Contributor

antonkomarev commented Sep 10, 2019

Me too, but it's an example what you can easily do with alpha ordered imports and what will require much more changes with length ordered imports.

In fact - I don't like to make changes in framework and check if my IDE isn't reordered imports before pushing the changes. For me it's much clear to have them ordered by name and to see all the imports from similar namespace near to each other and not to track down all the list to find what I want.

Is there at least one benefit to keep length ordered imports?

@driesvints
Copy link
Member Author

Is there at least one benefit to have length ordered imports?

More consistency with the rest of the PHP open source community.

@antonkomarev
Copy link
Contributor

antonkomarev commented Sep 10, 2019

@driesvints length ordered imports? really? I always thought that alpha ordered imports is standard for PHP.

Edit: I've changed "have" on "keep". I suppose it was a reason of your misread.

@driesvints
Copy link
Member Author

@antonkomarev ah sorry, I meant alpha ordered.

@taylorotwell taylorotwell merged commit 1bbe552 into 6.x Sep 10, 2019
@driesvints driesvints deleted the alpha-ordered-imports branch September 10, 2019 15:17
@browner12
Copy link
Contributor

The IDEs are gonna be pissed we hounded them for so long about making length ordering an option for auto-formatting, and now we go and just switch anyway. 😅

@devcircus
Copy link
Contributor

Strange move, as this exact change received so much push-back for years. Then suddenly, without explanation the change is made. To me, this was one of the many things that made the codebase a joy to peruse.

@martinbean
Copy link
Contributor

YES!

@HDVinnie
Copy link
Contributor

worst PR ever.... length ordered imports is so much cleaner looking.

@antonkomarev
Copy link
Contributor

And now they are easier to perceive. What better, pretty look or better readability?

@devcircus
Copy link
Contributor

If you have a decent ide, you never have to perceive anything. They just sit at the top of your classes. Before laravel I would have never sorted by length but after years of doing it that way, I’ve grown to love it.

@driesvints
Copy link
Member Author

Ordering alphabetically gives you a directory-like view of the imports which imo is a better 1-on-1 mapping when you browse a file directory. Ordering by length doesn't has any real benefit.

@netpok netpok mentioned this pull request Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants